The MutexGuard is held across an .await point in poll_event. Since wc.next().await can block indefinitely while waiting for events from the relay, this will prevent any other task from acquiring the lock. This effectively deadlocks other operations like request(), which also needs to acquire the same lock. Consider an architecture where the client is not locked during long-running awaits, or use a split reader/writer pattern if supported by the underlying library.

The wallet_transact tool registers a callback and returns a pending status, but it never actually initiates the transaction request via self.session.request(). This makes the tool non-functional as the wallet will never receive the signature request. The tool should initiate the request (potentially in a background task if it blocks) before returning the correlation ID.

Pin git dependencies to a specific commit hash (rev) for reproducible builds, as per the general rules of this repository.

When wc.next().await returns Ok(None), it indicates that the stream has ended. The current implementation returns None but leaves the client in the Option, causing the listener loop to enter a busy-wait state (polling every 100ms and getting None immediately). The client should be cleared (*client = None) when the stream ends to stop the polling.

Using format!("{a:?}") to extract the account address is unreliable as it depends on the Debug implementation of the AccountId type, which often includes the chain ID prefix (e.g., eip155:1:0x...). It is better to use a specific method to extract just the address string if available in the walletconnect-client API.

Submit wallet transaction before returning pending status

wallet_transact currently records callback metadata and returns a pending success payload, but never calls WalletConnectSession::request (or any other dispatch path) to actually send eth_sendTransaction. In this commit there is no production code path that consumes the correlation ID to submit the transaction, so users can receive a “submitted for approval” response while nothing was sent and the callback only times out.

Useful? React with 👍 / 👎.

Reset paired state when WalletConnect drops with error

When wc.next() returns WalletConnectError::Disconnected, the code clears wc_client but does not update status, so the session can remain Paired even though no client exists. In that state, pairing/transaction flows can behave inconsistently (e.g., appearing paired first, then failing later), and users may be blocked from cleanly re-pairing after a disconnect.

Useful? React with 👍 / 👎.

Critical — Pre-existing byte-slicing panic (same pattern)

This PR fixes byte-level truncation in test_server, but crates/ironclaw_tui/src/widgets/conversation.rs:863 has the same pattern: .truncate(57) which is byte-based and will panic on multi-byte UTF-8. The PR's own philosophy ("fix the pattern") demands fixing all instances.

Suggested fix: Also fix items_str.truncate(57) in conversation.rs to use chars().take() or floor_char_boundary(), matching the pattern used in this PR.

Fixed in dcbb7a7. The .truncate(57) in conversation.rs now uses char_indices() to find a safe truncation point, preventing panics on multi-byte UTF-8 characters.

High — Server-side API handler bypasses name normalization

src/channels/web/handlers/extensions.rs:143 passes req.name directly to ext_mgr.install() without normalization. The JS frontend normalizes on the client side, but direct API calls (e.g., curl) bypass the JS and send raw names. This creates client-server divergence.

Suggested fix: Apply normalize_server_name() on the server side in extensions_install_handler for mcp_server kind, or add name format validation to McpServerConfig::validate().

Fixed in dcbb7a7. The web API extensions_install_handler now applies normalize_server_name() to req.name before passing to ext_mgr.install().

Medium — Lookup commands don't normalize names

remove_server, auth_server, test_server, and toggle_server do NOT normalize the user-provided name. If a user adds "My Twitter Server" (stored as my_twitter_server), then runs ironclaw mcp test "My Twitter Server", the lookup fails with "Server not found" and no hint about the normalized name.

Suggested fix: Either normalize in lookup commands too (with a "did you mean 'my_twitter_server'?" fallback), or include the normalized form in the error message when lookup fails.

Medium — Tests duplicate logic instead of testing production code

The truncation regression tests (test_tool_description_truncation_multibyte_utf8, test_tool_description_truncation_ascii) duplicate the truncation logic inline rather than calling the actual production code path. If someone changes the production truncation logic (e.g., changes 57 to 50), these tests would still pass.

Suggested fix: Extract the truncation logic into a helper function used by both test_server and the tests, or write the tests to call test_server with a mock client.

Medium — Leading/trailing hyphens not trimmed

normalize_server_name trims leading/trailing underscores but not hyphens. Input "-hello-" produces "-hello-", and consecutive hyphens are not collapsed. Names like --- would produce ---, which could confuse downstream systems using the name in file paths or secret keys (e.g., mcp_---_access_token).

Suggested fix: Treat hyphens symmetrically: also collapse consecutive hyphens and trim leading/trailing hyphens, or document the intentional asymmetry. At minimum, add test cases for "-hello-", "---", and "a--b" to pin the behavior.

High Severity — CLI lookup commands don't normalize the name argument

add_server normalizes the name here, but the four other CLI sub-commands — remove_server (line 292), auth_server (line 418), test_server (line 502), and toggle_server (line 625) — all use the raw user-supplied name for servers.get(&name) / servers.remove(&name).

After this PR, ironclaw mcp add "My Server" https://... stores the server as my_server. But ironclaw mcp remove "My Server" would fail with "Server not found" because the stored name is my_server and the lookup is an exact string match.

Suggested fix: Add let name = normalize_server_name(&name); at the top of remove_server, auth_server, test_server, and toggle_server, matching what add_server does here.

Fixed in bc3f863. All four CLI lookup commands (remove_server, auth_server, test_server, toggle_server) now normalize the name argument before lookup, matching add_server behavior.

Medium Severity — Missing empty-name guard after normalization

normalize_server_name("@#$") returns "". The CLI's add_server checks for this (line 186–188) and returns an error, and the JS frontend checks if (!name) too. But this web handler passes the empty string straight to ext_mgr.install().

A direct API caller (bypassing the JS frontend) could create a server with an empty name.

Suggested fix:

let normalized_name = crate::tools::mcp::config::normalize_server_name(&req.name);
if normalized_name.is_empty() {
    return Ok(Json(ActionResponse::fail(
        "Server name contains no valid characters".to_string(),
    )));
}

Fixed in bc3f863. Added empty-name guard after normalization in the web API handler. Direct API callers sending names like "@#$" now get a clear error instead of creating a server with an empty name.